VariableOrderAccumulator #940
Conversation
| function Base.:(==)(vi1::VarInfo, vi2::VarInfo) | ||
| return (vi1.metadata == vi2.metadata && vi1.accs == vi2.accs) | ||
| end |
There was a problem hiding this comment.
In making this PR I learned that the default implementation for structs is
function Base.:(==)(vi1::VarInfo, vi2::VarInfo)
return (vi1.metadata === vi2.metadata && vi1.accs === vi2.accs)
endi.e. all the fields are compared with === even when calling ==. That was causing trouble with some tests that did == checks of comparing SimpleVarInfos. So note that before this PR e.g. VarInfo() != VarInfo(), and now VarInfo() == VarInfo().
Benchmark Report for Commit bf5eb42Computer InformationBenchmark Results |
| @@ -1808,13 +1800,12 @@ function BangBang.push!!(vi::VarInfo, vn::VarName, r, dist::Distribution) | |||
| [1:length(val)], | |||
| val, | |||
| [dist], | |||
| [get_num_produce(vi)], | |||
There was a problem hiding this comment.
This is a change in behaviour: Previously calling push!! automatically set the order for a variable. Now order is set only if the push!! takes place within tilde_assume!!. Options for this are
- say that it's the caller's responsibility to call
set_order!!afterpush!!. This could be fine because only ParticleGibbs cares aboutorder. - add an extra hook for accumulators for
push!!, that gets called on all accumulators on everypush!!call, so that they can adjust their state accordingly.
If this is only relevant for VariableOrderAccumulator then I'd lean towards 1. If it comes up with other accumulators too then 2. might be warranted.
Similar considerations apply to at least push!, merge, and subset, which after this PR might result in out-of-sync VariableOrderAccumulators.
There was a problem hiding this comment.
If this is only relevant for VariableOrderAccumulator then I'd lean towards 1.
I think that it's PG's responsibility to call setorder correctly, rather than DPPL, so I'd agree.
Similar considerations apply to at least push!, merge, and subset
Still think it should be handled in PG, not here. I assume that we could write functions like
function pg_push!!(...)
vi = push!!(...)
return setorder!!(...)
endand make sure to always use that in the PG code?
There was a problem hiding this comment.
I'm happy with that as long as it doesn't turn out that this is a common need for accumulators. One other instance comes to mind: Currently if you have a PointwiseLogDensityAccumulator in your varinfo and you subset or merge, the pointwise log densities don't get subsetted/merged, and you end up with an accumulator that tracks different variables from the varinfo. This is inconsequential because the use of PointwiseLogDensityAccumulator is so confined to calling the function that needs it.
I'm happy to make PG deal with this, but let's keep our eyes open in case this comes up with other accumulators.
There was a problem hiding this comment.
PointwiseLogDensityAccumulator in your varinfo and you subset or merge
Ah, I see -- this would be true in the past as well with PointwiseLogDensityContext tracking different things from the subsetted varinfo, right?
There was a problem hiding this comment.
Yep. I don't think PLDAccumulator by itself is a good enough argument for making these subset and merge functions, but it just made me wonder if this is a more common pattern with accumulators than we would at first assume. Easy to leave them out now and add them later if needed though.
There was a problem hiding this comment.
Previously calling
push!!automatically set theorderfor a variable. Now order is set only if thepush!!takes place withintilde_assume!!
Having looked at lots of this code in more detail recently, I don't think there is actually anywhere in the codebase that uses push!! outside of tilde_assume!!. (There are some tests, but we can trivially change the tests to match this new behaviour.) Do you know of any?
There was a problem hiding this comment.
Can easily believe that that's the only place.
|
Benchmark times indicate a horrendous loss of type stability. Will investigate, probably tomorrow. |
I had similar problems with other PRs. How about this?
|
|
Is there a particular reason to first drop it from default accumulators and then move it to Turing.jl, rather than doing both in one go? Also, regardless of what we do, I would develop the corresponding Turing.jl release in parallel, to avoid having to make a lot of patch DPPL releases when we realise we are missing something. I've started that work in TuringLang/Turing.jl#2550, but not yet for VariableOrderAccumulator. |
|
Because it's annoyingly difficult to make Turing CI run with an unreleased version of DPPL, short of committing a (I don't think patch releases are really problematic, but there is always the possibility of having to make multiple minor releases to fix bugs, so I see the point) |
|
The performance problem turned out to not be type stability, but rather that every call to Two thoughts:
|
|
I'm just going through my list of supposed-to-review PRs and clearing them. Feel free to ping me again whenever you feel this is ready |
|
DynamicPPL.jl documentation for PR #940 is available at: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #940 +/- ##
============================================
- Coverage 82.58% 82.07% -0.51%
============================================
Files 38 38
Lines 4007 4023 +16
============================================
- Hits 3309 3302 -7
- Misses 698 721 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -1808,13 +1800,12 @@ function BangBang.push!!(vi::VarInfo, vn::VarName, r, dist::Distribution) | |||
| [1:length(val)], | |||
| val, | |||
| [dist], | |||
| [get_num_produce(vi)], | |||
There was a problem hiding this comment.
Previously calling
push!!automatically set theorderfor a variable. Now order is set only if thepush!!takes place withintilde_assume!!
Having looked at lots of this code in more detail recently, I don't think there is actually anywhere in the codebase that uses push!! outside of tilde_assume!!. (There are some tests, but we can trivially change the tests to match this new behaviour.) Do you know of any?
|
Good spot, fixed |
|
For ease of comparison, here's the latest benchmark run: And here's the same thing from before these changes, on There are substantial though not massive slowdowns across the board, and an especially significant hit for a model with a lot of varnames. I think this is fine once we make sure that |
This should have been changed in #940, but slipped through as the file wasn't listed as one of the changed files.
Removes the
orderfield ofMetadatain favour of having anOrderedDict{VarName,Int}in the same accumulator asnum_produce(renamingNumProduceAccumulatortoVariableOrderAccumulatorin the process). Also adds some==methods we were previously missing.This is currently passing tests except anything related to JET. I think JET freaks out because the
OrderedDictwithin the new accumulator has an abstract key type. I think it's fine to have the abstract key type as long as the value type is concrete, at least once we removeVariableOrderAccumulatorfrom the set of default accumulators and only use it when doing ParticleGibbs. I'm thus tempted to not fix the JET issues and move this whole accumulator from DPPL to Turing.jl's part that interfaces with AdvancedPS. Not sure how to handle merging this PR in that case though.